Repository navigation
fix(security): prototype pollution, HTML text and ReDoS backlog (CodeQL) - #187
Merged
Merged
Conversation
Close the production-code CodeQL alerts open on main since the first scan: 11, 12, 13, 16, 17, 19, 20, 23, 24, 30, 32, 35, 36, 41, 42, 43, 44 and 45. - Prototype pollution: A2UI data paths refuse __proto__, constructor and prototype at every level with an InteropError, write own properties only (defineProperty) and step into arrays by index only. Surface binding pointers with those keys are a checkSurface issue, the Build eval's sample inputs skip them, and set_chrome / update_entry's fields merge refuse them. RESERVED_KEYS and isReservedKey in core-ts/names. - htmlText never returns markup: a linear tag strip, entities decoded once, and a `<` that would open markup becomes U+FF1C. Every sink escapes too (Astro attributes, jsonLdText, the feeds' xml()). - ReDoS (AGENTS.md rule 13): linear scanners for the svg sniff (exponential on uploads), fence opener and closer (no RegExp built from body text), field markdown conversions, hostname normalization, leading H1, table of contents, ::embed check, sender address (isMailAddress in @cloudhal/api; MAIL_ADDRESS deprecated), Studio's origin trim and the editor's paste link check. Each has an equivalence test against the old regex (cases plus seeded random strings) and 100k adversarial inputs. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Madraka <[email protected]>
- htmlText returns the decoded text verbatim again (no U+FF1C rewrite: "Array<string>" and "x<y" read as written). Safety is each sink's, now tested one by one with <script>, <!-- and a quote breakout, parsed with parse5: meta description, og: and twitter: attributes, JSON-LD (archive and correction notes, jsonLdText), the archive's RSS description, table-of-contents labels and search snippets. Every sink already escaped; none needed a fix. - The reserved-key check in surface pointers applies only where the page writes (input value/checked, Scrolly step, Button action path), so read bindings through a data key named constructor or prototype stay valid. - An A2UI input whose component id is a reserved key gets the data key `<id>-value` instead of failing the conversion. - leadingHeading matches the old regex exactly again (#-only and blank lines after the H1, `#` followed by a line break, and lone \r as tail whitespace), checked by a seeded random comparison over #, space, \n, \r, x, \t and . Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Madraka <[email protected]>
Move the A2UI fallback data-key rule into surface-interop-data.ts (dataKeyFor), so fromA2ui and surface-interop.ts do not grow. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Madraka <[email protected]>
…id cases - html-text-sinks: compare against JSON.stringify(DECODED) instead of a quote-only replace (CodeQL alert 52), and check Studio's archive preview: its description meta (esc()) holds the decoded text as an attribute value, and the page parses to no script, img, svg or comment. - interop-pollution: an A2UI input whose component id is __proto__ converts, folded to the plain id and data key "proto". Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Madraka <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the production-code CodeQL alerts open on
mainsince the first scan (#160): prototype pollution (42–44), the HTML-text sanitizer (41) and the polynomial/exponential ReDoS backlog (11, 12, 13, 16, 17, 19, 20, 23, 24, 30, 32, 35, 36), plus alert 45, whose real sink was insurfaces/body.ts. The Actions and test/script alerts follow in a second PR.Prototype pollution (alerts 42, 43, 44)
pointerSetin@cloudhal/core-ts/interop(A2UIupdateDataModel, reachable from agent and MCP input) skipped reserved keys silently and wrote throughcur[s]. It now lives insurface-interop-data.tsand:__proto__,constructorandprototypeat every level with anInteropError(not a silent skip); the check is inline at each step, in front of every write;Object.hasOwnand writes withObject.defineProperty(own data properties only, never a setter or the prototype chain); deletes only own keys;/list/lengthis an error).Other setters found by grepping:
/__proto__/x, and the Build eval'swithSampleInputs(packages/api/src/build-eval.ts) writes through binding paths withat[k] = {}— a real pollution path from model output.checkSurfacenow reports reserved keys in pointers the page writes through (an input'svalue/checked, a Scrolly'sstep, a Button action'spath;checkWrittenPointer); read bindings may still nameconstructororprototype(they go through the own-onlygetPointer), so existing pages keep saving.withSampleInputsskips reserved keys and reads own properties only.prototype/constructornow gets<id>-valueinstead of failing the conversion (dataKeyFor).set_chrome's part merge andupdate_entry'sfieldsmerge assignedout[k] = vfor keys from JSON input. A JSON"__proto__"key there swaps the object's prototype (not global, but wrong). Both now refuse reserved keys withToolError("invalid").applyPatch/patchPage) already refused reserved keys with an error and usedhasOwn; it now sharesRESERVED_KEYSfrom@cloudhal/core-ts/names(new, withisReservedKey).surface-client.ts), which already skips reserved keys and only ever writes the page's own data.Tests (
core-ts/test/interop-pollution.test.ts,mcp-serverchrome and fields tests,api/test/design-pass.test.ts):/__proto__/x,/__proto__,/constructor/prototype/x, nested and~1-encoded forms, deletes; each asserts the error and that({}).xstays undefined.htmlText(alert 41)htmlTextstripped tags with/<[^>]*>/(also a rule-13 pattern) and decoded entities afterwards, so<script>became<script>. Callers and sinks, traced:markdownText→archiveHeaddescription<meta name="description" content>,og:/twitter:tags (SeoHead.astro attributes), JSON-LDjsonLdTextescapes<,>,&markdownText→archiveFeedXml<description>xml()inlineMarkdownText→ correction notesCorrectionComment.textjsonLdTexttableOfContentsArticle.astro{item.text})set:htmlitheadingSlug[a-z0-9-]readingMinutesDecision (after review):
htmlTextreturns plain text, verbatim (Array<string>readsArray<string>; the first revision's U+FF1C rewrite changed visible text and is gone). Safety is each sink's, andtest/html-text-sinks.test.tsproves it sink by sink:<script>alert(1)</script> <!-- c --> <img onerror> "><svg onload>goes through each, and the output is parsed with parse5, which must find no element or comment beyond the sink's own:<meta name=description>,og:*,twitter:*addAttribute(&,")<meta>;contentround-tripsjsonLdText(<>&as\u003c…: no</script>, no<!--)<script>element; JSON round-trips<description>xml()<script/<img/<svg/<!--{item.text})escapeHTML<a>htmlTextcaller;innerHTML/set:html)snippetHtml<mark>llms.txtdoes not callhtmlText: it is Markdown served astext/plain. No sink needed a fix. The tag strip is still the linear scanner, and entities are decoded once (&#60;is the text<; the old chain decoded it twice). If CodeQL still reportshtmlTextafter merge, that one alert is dismissed with this table as the reason.ReDoS (rule 13)
Each rewrite is a linear scanner with an equivalence test against the old regex (hand-picked cases plus 20k seeded random strings over the characters that matter, or every short string over a small alphabet) and 100k-character adversarial inputs under 1.5 s.
core-ts/src/media/sniff.tsisSvgText(uploads)<!--+--><!--×n (22 repeats ≈ 1.7 s)-->(the regex could stretch one past it: only ever answers "not svg")ui/src/blocks/editor/paste.tslink check[×n,[!](×nhasInlineLink: each](once, target scans sharedcore-ts/src/surfaces/body.tsfence opener; closer RegExp built from body text\r; CodeQL traced the line intonew RegExp(alert 45's "used as a regular expression")fenceOpen,closesFencescanners, no dynamic RegExpcore-ts/src/fields/conversions.tsmarkdownToPlainlinks/imagesescapeMarkdown^(\s*)…/gmescapeAtLineStartscore-ts/src/domains.tsnormalizeHostname[/?#].*$cutPathPartcloudflare/src/mail.tsMAIL_ADDRESS.xrunsisMailAddress(new in@cloudhal/api);MAIL_ADDRESSkept, deprecated. The CLI's copy of the regex (config.ts) gets the same linear check locally, so readingcloudhal.jsondoes not load the APIastro-renderer/src/markdown.tstag strip<×nstripTagsmarkdown.tsleading H1leadingHeading, exactly the old regex's match (incl.#-only and blank lines after the H1); seeded random comparison over#, space,\n,\r,x,\t,\u2028, plus 3.5M strings offline: 0 differencesastro-renderer/src/kit.tstable of contents<h2 id>×nrenderedHeadings, closer searched once per depthastro-renderer/src/embed-client.ts::embedcheckhasEmbedLineapi/src/tokens.tsstudioOrigin/\/+$/trimEndCharChecks
pnpm exec vitest runper package, one at a time, all green: core-ts 63 files / 1122 tests, astro-renderer 50 / 363, mcp-server 24 / 176, ui 18 / 216, api 45 / 617, cloudflare 14 / 57, cli 63 / 446. (A first core-ts run alongside other suites dropped Miniflare D1 connections in the D1 halves of the store suites; the re-run on its own passed.)pnpm lint(warnings only, as on main),pnpm size:check(surface-interop.ts shrank 1233 → 1192; baseline tightened),pnpm design:lint,tsc --noEmitin all seven packages.Patch changeset for core-ts, astro-renderer, api, mcp-server, cloudflare and cli (
@cloudhal/uiis private).Follow-up (not in this PR)
core-ts/src/structured-data/values.tsbodyText(HTML bodies → JSON-LDarticleBody) has the same strip-then-decode shape and a lazy<script>…</script>scan; CodeQL did not flag it and its sink (jsonLdText) escapes, but it should use the same scanners.🤖 Generated with Claude Code